Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Listen for removed inputs and unregister them #67

Merged
merged 3 commits into from
Aug 15, 2023

Conversation

LiteracyFanatic
Copy link
Contributor

This addresses issue #66 by looping through the MutationRecord's removedNodes array and removing entries for any validatable elements from the various state objects. This appears to fix the issue with isValid and isFieldValid for my test case, but there are a couple parts I'm unsure of:

  • I had to add a check for undefined validators in getFormValidationTask which seems to indicate that this.formInputs[formUID] still contains entries for removed inputs even though the splice call in untrackFormInput should have removed them.
  • Not sure if it's cleaner to use the removed boolean that's currently being passed to scan and its children or to have a separate set of functions instead

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@dahlbyk dahlbyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines +676 to +679
const validator = this.validators[inputUID];
if (validator) {
formValidators.push(validator);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree it's curious that inputUID doesn't seem to have been deleted, but guarding against undefined also doesn't both me.

@haacked haacked merged commit 65e97ce into haacked:main Aug 15, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants